Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ztimer/periodic: reinit remove from right clock and handle aquired ztimer #19826

Merged
merged 1 commit into from
Jul 24, 2023
Merged

ztimer/periodic: reinit remove from right clock and handle aquired ztimer #19826

merged 1 commit into from
Jul 24, 2023
+14 −18

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Jul 13, 2023 •
edited
Loading

Contribution description

#19806 added some retinit handling for ztimer periodic removing the timer from the new clock

This tries to detect if this is a reinit and remove the timer from the old clock
this also removes the ztimer_acquire/_release handling by removing now calls in favour of set return value and now values that are allready in ztimer,
that also has the potential to reduce the jitter of the periodic calls and bus-usage (for cpus that take their time to get "now")

Testing procedure

read

run tests/sys/ztimer_periodic

Issues/PRs references

Fixes #19806

@kfessel kfessel requested review from benpicco and removed request for kaspar030 and bergzand July 13, 2023 14:19
@github-actions github-actions bot added Area: timers Area: timer subsystems Area: sys Area: System labels Jul 13, 2023
@kfessel kfessel requested review from kaspar030 and bergzand July 13, 2023 14:20
@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 13, 2023
@riot-ci
Copy link

riot-ci commented Jul 13, 2023 •
edited
Loading

Murdock results

✔️ PASSED

4f2237d ztimer/periodic: stop timer before reinit, remove aquire/release

Success Failures Total Runtime
6936 0 6936 09m:21s

Artifacts

@kfessel kfessel force-pushed the p-ztimer_periodic_init branch 3 times, most recently from ba1d5f9 to c4c555f Compare July 14, 2023 10:22
@kfessel kfessel added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 14, 2023
@kfessel
Copy link
Contributor Author

kfessel commented Jul 14, 2023

upgraded this to bug, since restart is documented but leaves a wrong ztimer_aquire/release count without this PR (since it isn't tracking correctly unless the same number for starts and stops happen)

RIOT/sys/include/ztimer/periodic.h

Line 122 in b482a71

* When called on an already running timer, the current interval is reset to its

benpicco
benpicco reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, but uncrustify is not happy (and rightfully so)

@kfessel kfessel force-pushed the p-ztimer_periodic_init branch from c4c555f to abecdd0 Compare July 14, 2023 11:05
benpicco
benpicco reviewed Jul 14, 2023
View reviewed changes
sys/ztimer/periodic.c Outdated
Comment on lines 87 to 89
ztimer_remove(timer->clock, &timer->timer);
ztimer_release(timer->clock);
timer->timer.arg = NULL;
Copy link
Contributor

@benpicco benpicco Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also do this (other than the ztimer_remove()) inside _ztimer_periodic_callback() for the !ZTIMER_PERIODIC_KEEP_GOING case?

Why is ztimer_acquire()/ztimer_release() needed here when we do a ztimer_set() anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is ztimer_acquire()/ztimer_release() needed here when we do a ztimer_set() anyway?

atm it needs the aquire (at least for start), but maybe we can rewrite it

@kfessel kfessel force-pushed the p-ztimer_periodic_init branch from abecdd0 to 957021e Compare July 14, 2023 12:51
@kfessel
Copy link
Contributor Author

kfessel commented Jul 14, 2023 •
edited
Loading

How about this

but now it should be tested

did that -> failed added + timer->interval at the start

@kfessel kfessel force-pushed the p-ztimer_periodic_init branch from 957021e to a2054e8 Compare July 14, 2023 13:21
@kfessel kfessel added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CI Area: Continuous Integration of RIOT components and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CI Area: Continuous Integration of RIOT components labels Jul 14, 2023
@kfessel kfessel force-pushed the p-ztimer_periodic_init branch 2 times, most recently from b9ab8aa to 1cb9bcf Compare July 14, 2023 14:55
@kfessel
ztimer/periodic: stop timer before reinit, remove aquire/release
4f2237d 
    - not ztimer_now calls outsid irq -> remove aquire
@kfessel kfessel force-pushed the p-ztimer_periodic_init branch from 1cb9bcf to 4f2237d Compare July 14, 2023 16:27
kaspar030
kaspar030 approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine here generally, didn't do any testing.

ACK

benpicco
benpicco reviewed Jul 24, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors merge

@bors
Copy link
Contributor

bors bot commented Jul 24, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit bff7452 into RIOT-OS:master Jul 24, 2023
@kfessel kfessel mentioned this pull request Aug 1, 2023
Closed
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco left review comments

@kaspar030 kaspar030 kaspar030 approved these changes

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

Assignees
No one assigned
Labels
Area: sys Area: System Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.10
Development

Successfully merging this pull request may close these issues.
5 participants
@kfessel @riot-ci @benpicco @kaspar030 @MrKevinWeiss